Add NettyByteBuf to GetBlobOperation #1314

justinlin-linkedin · 2019-11-20T01:27:10Z

This starts to use Netty ByteBuf in GetBlobOperation.

This PR seeks the least intrusive way to introduce Netty ByteBuf to GetBlobOperation, so it ignores a lot of places where the ByteBuffer can be replaced by Netty ByteBuf.

The major change is to change the buffer/inputstream in BlobData to use Netty ByteBuf.

codecov-io · 2019-11-21T02:01:53Z

Codecov Report

Merging #1314 into master will decrease coverage by 0.03%.
The diff coverage is 70.73%.

@@             Coverage Diff              @@
##             master    #1314      +/-   ##
============================================
- Coverage     72.04%   72.01%   -0.04%     
- Complexity     6699     6714      +15     
============================================
  Files           486      486              
  Lines         38163    38207      +44     
  Branches       4843     4843              
============================================
+ Hits          27495    27513      +18     
- Misses         9352     9382      +30     
+ Partials       1316     1312       -4

Impacted Files	Coverage Δ	Complexity Δ
...ava/com.github.ambry/store/HardDeleteVerifier.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../com.github.ambry/tools/admin/ServerAdminTool.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...github.ambry/tools/perf/ServerReadPerformance.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...va/com.github.ambry/tools/admin/BlobValidator.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...hub.ambry.messageformat/ValidatingTransformer.java	`90% <100%> (-0.25%)`	`5 <0> (ø)`
...in/java/com.github.ambry/network/ResponseInfo.java	`90% <100%> (+3.63%)`	`10 <0> (-1)`	⬇️
...om.github.ambry.replication/BlobIdTransformer.java	`95.08% <100%> (ø)`	`22 <0> (ø)`	⬇️
.../com.github.ambry.utils/ByteBufferInputStream.java	`94.73% <100%> (+3.24%)`	`18 <0> (-1)`	⬇️
.../main/java/com.github.ambry.router/DecryptJob.java	`94.59% <100%> (+0.3%)`	`6 <0> (+1)`	⬆️
...ithub.ambry.messageformat/MessageFormatRecord.java	`82.17% <100%> (ø)`	`29 <4> (+1)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e2b8dd...81fb3a6. Read the comment docs.

jsjtzyy

Mostly looks good. A few comments.

jsjtzyy · 2019-11-22T00:05:05Z

ambry-router/src/main/java/com.github.ambry.router/GetBlobOperation.java

@@ -1197,18 +1190,15 @@ void handleBody(ResponseInfo responseInfo, InputStream payload, MessageMetadata
          }
        }
        blobType = blobData.getBlobType();
-        chunkIndexToBuffer = new TreeMap<>();
-        chunkIndexToResponseInfo = new ConcurrentHashMap<>();
+        chunkIndexToBuf = new ConcurrentHashMap<>();


Does this need to be ConcurrentHashMap?

We will remove ByteBuf from this hash map in other threads, so at least it has to be thread safe.

jsjtzyy · 2019-11-22T00:25:32Z

ambry-router/src/main/java/com.github.ambry.router/GetBlobOperation.java

-        ByteBuffer chunkBuffer = blobData.getStream().getByteBuffer();
-        responseInfo.retain();
-        chunkIndexToResponseInfo.put(chunkIndex, responseInfo);
+        ByteBuf chunkBuffer = blobData.getAndRelease();


nit: let's call it chunkBuf

jsjtzyy · 2019-11-22T00:30:57Z

ambry-router/src/main/java/com.github.ambry.router/GetBlobOperation.java

@@ -838,7 +836,7 @@ void handleResponse(ResponseInfo responseInfo, GetResponse getResponse) {
     * @param targetBlobId the {@link BlobId} of the blob.
     * @return {@code true} if a crypto job was launched, otherwise {@code false}.
     */
-    protected boolean maybeLaunchCryptoJob(ByteBuffer dataBuffer, byte[] userMetadata, ByteBuffer encryptionKey,
+    protected boolean maybeLaunchCryptoJob(ByteBuf dataBuffer, byte[] userMetadata, ByteBuffer encryptionKey,


How about making variable naming consistent, if its type is ByteBuf, we call it **Buf?

jsjtzyy · 2019-11-22T01:26:02Z

ambry-router/src/main/java/com.github.ambry.router/GetBlobOperation.java

@@ -857,7 +855,7 @@ protected boolean maybeLaunchCryptoJob(ByteBuffer dataBuffer, byte[] userMetadat
        decryptCallbackResultInfo = new DecryptCallBackResultInfo();
        progressTracker.initializeCryptoJobTracker(CryptoJobType.DECRYPTION);
        decryptJobMetricsTracker.onJobSubmission();
-        cryptoJobHandler.submitJob(new DecryptJob(targetBlobId, encryptionKey, dataBuffer,
+        cryptoJobHandler.submitJob(new DecryptJob(targetBlobId, encryptionKey, dataBuffer.retainedDuplicate(),


I don't see the dataBuffer is released in this method. (Correct me if I misunderstood)

dataBuffer is not created in this function, so we don't release it here. But it will be released in the DecryptJob.

jsjtzyy · 2019-11-22T01:39:42Z

ambry-messageformat/src/main/java/com.github.ambry.messageformat/BlobData.java

    return stream;
  }
+
+  /**
+   * Return the netty {@link ByteBuf} and then transfer the ownship to the caller. It's not safe


typo: ownership. And why it's not safe to call this more than once? You have byteBuf == null check here. Caller is supposed to check result.

Since this is not an reentrant method, calling this method multiple time would get you multiple result so it's better not to call it twice.

ownship -> ownership

jsjtzyy · 2019-11-22T01:46:05Z

ambry-messageformat/src/main/java/com.github.ambry.messageformat/BlobData.java

+      return null;
+    }
+    ByteBuffer temp = ByteBuffer.allocate(byteBuf.readableBytes());
+    byteBuf.writeBytes(temp);


Why writeBytes here? I feel like it's supposed to be readBytes from byteBuf to temp, right?

you are right, updated.

ambry-replication/src/main/java/com.github.ambry.replication/BlobIdTransformer.java

jsjtzyy · 2019-11-22T02:45:48Z

ambry-utils/src/main/java/com.github.ambry.utils/Utils.java

+    }
+    output.flip();
+    return output;
+  }


I feel like this method can be replaced by ByteBufferInputStream and call getByteBuffer() to obtain the byteBuffer. At least we can consider reusing the code in the ctor of ByteBufferInputStream.

...messageformat/src/test/java/com.github.ambry.messageformat/MessageFormatInputStreamTest.java

...essageformat/src/test/java/com.github.ambry.messageformat/MessageSievingInputStreamTest.java

cgtz · 2019-11-27T23:17:03Z

ambry-api/src/main/java/com.github.ambry/network/ResponseInfo.java

  /**
   * Decrease the reference count of underlying response.
   */
  public void release() {
    if (response != null) {
      ReferenceCountUtil.release(response);
+      response = null;


Should this only be set to null when the refcount reaches zero?

now that we remove the retain function, we can be sure that we only need to release it once.

ambry-messageformat/src/main/java/com.github.ambry.messageformat/MessageFormatRecord.java

cgtz

I can merge after these comments are addressed

cgtz · 2020-01-31T01:50:56Z

ambry-messageformat/src/main/java/com.github.ambry.messageformat/BlobData.java

    return stream;
  }
+
+  /**
+   * Return the netty {@link ByteBuf} and then transfer the ownship to the caller. It's not safe


ownship -> ownership

cgtz · 2020-01-31T01:58:49Z

ambry-router/src/main/java/com.github.ambry.router/DecryptJob.java

@@ -73,14 +74,18 @@ public void run() {
      Object containerKey = kms.getKey(blobId.getAccountId(), blobId.getContainerId());
      Object perBlobKey = cryptoService.decryptKey(encryptedPerBlobKey, containerKey);
      if (encryptedBlobContent != null) {
-        decryptedBlobContent = cryptoService.decrypt(encryptedBlobContent, perBlobKey);
+        decryptedBlobContent = cryptoService.decrypt(encryptedBlobContent.nioBuffer(), perBlobKey);


Why not use the CryptoService.decrypt(ByteBuf, Key) method here? I think the default method has some special handling for edge cases with composite buffers.

planning on call the other method in a different PR, where I will change bytebuffer in DecryptJob and EncryptJob to ByteBuf.

ambry-router/src/main/java/com.github.ambry.router/GetBlobOperation.java

ambry-router/src/test/java/com.github.ambry.router/GetBlobOperationTest.java

build.gradle

ambry-utils/src/main/java/com.github.ambry.utils/Utils.java

justinlin-linkedin requested review from cgtz and jsjtzyy November 20, 2019 01:27

justinlin-linkedin force-pushed the getopnetty branch from 1994da0 to bbe3cc4 Compare November 20, 2019 23:47

jsjtzyy reviewed Nov 22, 2019

View reviewed changes

justinlin-linkedin force-pushed the getopnetty branch 2 times, most recently from 06f5bc5 to 3d31576 Compare November 25, 2019 23:35

cgtz reviewed Dec 2, 2019

View reviewed changes

justinlin-linkedin force-pushed the getopnetty branch from 3d31576 to 4437544 Compare December 5, 2019 01:42

justinlin-linkedin force-pushed the getopnetty branch from 4437544 to 5028916 Compare December 17, 2019 18:18

justinlin-linkedin force-pushed the getopnetty branch 2 times, most recently from 97cdcf9 to 52b959f Compare January 9, 2020 20:53

justinlin-linkedin force-pushed the getopnetty branch from 52b959f to bb98acb Compare January 15, 2020 17:47

justinlin-linkedin force-pushed the getopnetty branch from bb98acb to 0571c0a Compare January 27, 2020 18:46

jsjtzyy approved these changes Jan 27, 2020

View reviewed changes

justinlin-linkedin added 3 commits January 30, 2020 16:07

Start using Netty ByteBuf in GetBlobOperation

63cb660

fix

cbc5dd6

Rebase

54f9606

justinlin-linkedin force-pushed the getopnetty branch from 0571c0a to 54f9606 Compare January 31, 2020 00:07

justinlin-linkedin added 2 commits January 30, 2020 16:20

Fix some comments

b1c48ce

Fix test error

0adb3fb

cgtz suggested changes Jan 31, 2020

View reviewed changes

cgtz reviewed Jan 31, 2020

View reviewed changes

build.gradle Show resolved Hide resolved

cgtz reviewed Jan 31, 2020

View reviewed changes

ambry-utils/src/main/java/com.github.ambry.utils/Utils.java Show resolved Hide resolved

Comments

81fb3a6

cgtz approved these changes Jan 31, 2020

View reviewed changes

cgtz merged commit 3cdb994 into linkedin:master Jan 31, 2020

justinlin-linkedin deleted the getopnetty branch January 31, 2020 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NettyByteBuf to GetBlobOperation #1314

Add NettyByteBuf to GetBlobOperation #1314

justinlin-linkedin commented Nov 20, 2019

codecov-io commented Nov 21, 2019 •

edited

Loading

jsjtzyy left a comment

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019 •

edited

Loading

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019

cgtz Jan 31, 2020

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 25, 2019

cgtz Nov 27, 2019

justinlin-linkedin Dec 5, 2019

cgtz left a comment

cgtz Jan 31, 2020

cgtz Jan 31, 2020

justinlin-linkedin Jan 31, 2020

Add NettyByteBuf to GetBlobOperation #1314

Add NettyByteBuf to GetBlobOperation #1314

Conversation

justinlin-linkedin commented Nov 20, 2019

codecov-io commented Nov 21, 2019 • edited Loading

Codecov Report

jsjtzyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinlin-linkedin Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 21, 2019 •

edited

Loading

justinlin-linkedin Nov 25, 2019 •

edited

Loading